Repository navigation
DXBE-8: Add source:link, source:unlink, source:cms:config:pull and source:cms:config:push commands - #2035
DXBE-8: Add source:link, source:unlink, source:cms:config:pull and source:cms:config:push commands#2035phenaproxima wants to merge 39 commits into
source:link, source:unlink, source:cms:config:pull and source:cms:config:push commands#2035Conversation
Adds a new source:config:push command that assembles the config files under .acquia/config into a single YAML payload (keyed by config collection, then config name) and POSTs it to the Sites Aggregation Service (SAS), polling the resulting async operation until completion. Introduces a SasApi client layer modeled on the existing AcsfApi pattern. Because SAS shares the Accounts authentication layer with the Cloud API, the connector reuses the standard OAuth2 client-credentials token flow; only the base URI is new (ACLI_SAS_API_BASE_URI). Open items are marked with @todo DXBE-20: the SAS endpoint path and response field names are placeholders pending the SAS endpoint being built, and the payload may need to be JSON-encoded if the SAS team requires it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2035 +/- ##
============================================
+ Coverage 92.77% 93.12% +0.35%
- Complexity 2037 2106 +69
============================================
Files 126 133 +7
Lines 7345 7538 +193
============================================
+ Hits 6814 7020 +206
+ Misses 531 518 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new source:config:push Symfony Console command to package local .acquia/config/**/*.yml into a single “collections → config name → values” document and submit it to a new SAS (Sites Aggregation Service) API client layer, with polling for async completion. This fits alongside the existing Cloud/Acsf client patterns and command set.
Changes:
- Introduces
source:config:pushcommand, payload assembly, confirmation prompt, and async polling. - Adds a new
SasApi/client layer (connector, client service, endpoint wrapper) and wires it into the production service container. - Adds unit tests for payload assembly behavior (collections and empty cases).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/src/Commands/Source/ConfigPushCommandTest.php | Adds unit tests for config payload assembly from .acquia/config directory structure. |
| src/Command/Source/ConfigPushCommand.php | Implements the source:config:push command: reads YAML config files, submits to SAS, and polls operation status. |
| src/SasApi/SourceConfig.php | Defines SAS endpoint wrapper methods for submitting a config push and checking operation status. |
| src/SasApi/SasCredentials.php | Provides SAS base URI configuration (env var + default). |
| src/SasApi/SasConnectorFactory.php | Adds a connector factory for SAS requests. |
| src/SasApi/SasConnector.php | Adds a SAS connector class extending the Cloud API connector. |
| src/SasApi/SasClientService.php | Adds a client-service factory for producing configured SasClient instances. |
| src/SasApi/SasClient.php | Adds a SAS client class extending the Cloud API client. |
| config/prod/services.yml | Registers SAS credentials + connector factory wiring and excludes SourceConfig from auto-service registration (instantiated manually by the command). |
Suppressed comments (2)
src/Command/Source/ConfigPushCommand.php:93
execute()currently callsdetermineSiteInstance(), which only returns a value when--siteInstanceIdis provided (seeCommandBase::determineSiteInstance()), so the command will always throw in the default “no args” flow and the[environmentId]argument is effectively ignored. UsingCommandBase::determineEnvironment()here would allow the documented environment resolution (including git-remote inference) while still supporting--siteInstanceIdas an override.
$siteInstance = $this->determineSiteInstance($input);
if ($siteInstance === null) {
throw new AcquiaCliException(
'Could not determine a Source site instance. Run this command from a repository linked to an Acquia Cloud application, or pass --siteInstanceId.'
);
src/Command/Source/ConfigPushCommand.php:157
Yaml::parseFile()will throw on invalid YAML and can return non-array values; right now that bubbles up as an unhandled exception or produces an invalid payload shape. Consider catching YAML parse failures per-file and throwing anAcquiaCliExceptionthat includes the offending path, and validate that each config file parses to an array/map.
$collection = $relativeDir === '' ? '' : str_replace('/', '.', $relativeDir);
$name = $file->getBasename('.yml');
$payload[$collection][$name] = Yaml::parseFile($file->getPathname());
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| protected function execute(InputInterface $input, OutputInterface $output): int | ||
| { | ||
| $this->setDirAndRequireProjectCwd($input); | ||
|
|
There was a problem hiding this comment.
Fair point on the convention, but we're deliberately holding off on an execute()-level test for now: the SAS endpoint doesn't exist yet, so any test would just cement a placeholder request/response shape we'd have to redo once the real API lands. The payload assembly (the novel logic) is covered. We'll add command-level coverage once the endpoint's contract is settled — tracked as part of DXBE-20.
Mirror the Cloud API ConnectorFactory: fall back to an AccessTokenConnector when a valid access token is present (e.g. a bot token in CI), instead of always building a key/secret connector. Also widen the SasConnector config phpdoc to allow nullable values.
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/2035/acli.phar |
CodebaseEnvironmentResponse exposes ->id, unlike EnvironmentResponse which uses ->uuid. Passing the wrong property would have errored at runtime.
The SAS endpoint triggers drush source:config:import, which reads config from the site's deployed git repository. The acli command is a thin trigger: resolve the site instance, POST with an empty body, poll. Remove the payload-assembly logic and its tests, which are no longer needed.
Adds source:config:pull, a mirror of push that triggers a SAS config export (CMS to repo). Extracts the shared trigger/poll flow into an abstract ConfigCommandBase so both directions reuse the same SAS client wiring; each subclass supplies only its endpoint call and messaging. Generalizes SourceConfig::getStatus() to serve both operations.
Pull now fetches the exported YAML payload after the operation completes and writes it to .acquia/config/, wiping and rewriting the directory so local files mirror the remote state exactly (config removed in the CMS disappears locally). Collections map to directories (default '' is the root; language.es becomes language/es). Adds unit tests for the writer. Adds SourceConfig::getExportPayload() to retrieve the YAML, and reworks ConfigCommandBase with an onSuccess() hook so pull can write files after a successful operation while push stays trigger-only.
Mock the Cloud API site-instance resolution chain and the SAS client to exercise the full execute() path for both push and pull: trigger, poll, and (for pull) payload fetch and write. Endpoint shapes are placeholders (@todo DXBE-20) to be re-pointed once the real SAS endpoint lands.
Add unit tests covering previously-escaped mutants: SasConnectorFactory connector selection (key/secret vs valid/expired token vs none), SasConnector base-URI passthrough, and SasClientService construction. Strengthen the push execute() test to assert exact status output, and add a nested-structure writer test to kill the Yaml::dump depth/indent mutants. Mark the transient spinner-message concat as infection-ignored (it never appears in captured output, so it cannot be asserted).
Rework the non-array-collection test so the malformed entry is iterated between two valid collections, catching a continue-to-break mutation while staying alphabetical for the code-style fixer. Add an empty-payload test to kill the mkdir-removal mutant. Mark two genuinely unobservable framework-glue mutants (onSuccess visibility, configureClient headers) as infection-ignored with justification.
Extract the Yaml::dump magic numbers into named constants and mark the unobservable depth increment/decrement as infection-ignored. Fix the access-token factory test to assert the connector type rather than the token value (the existing Cloud code nests the token object, a pre-existing quirk not worth depending on).
Add partial-credential-plus-token cases so a flipped key/secret condition (&& mutated to ||, or a negated operand) routes to the wrong branch and fails the test. This makes the factory's auth-selection logic observable at the unit level.
The key/secret branch and the unauthenticated fallback both return a SasConnector, so removing the branch's return produced an identical type. Distinguish them by the connector's private clientId: 'k' on the authenticated path, null on the fallback.
Both the key/secret branch and the unauthenticated fallback construct a SasConnector from the same config, so removing the branch's return yields an externally identical object. Mark it infection-ignored with justification, and remove the reflection-based test that could not distinguish the branches. Local Infection reports 100% MSI.
This AFAICT needs both |
|
This PR is correctly hard-blocked on SAS + SAT PRs (see prior comment), but AFAICT neither PR currently does enough to allow this PR to work: https://github.com/acquia/sites-aggregation-service/pull/971#pullrequestreview-4960954310 |
| */ | ||
| public function push(string $environmentId): object | ||
| { | ||
| return $this->client->request('post', "/environments/$environmentId/config-import"); |
There was a problem hiding this comment.
acquia/sites-aggregation-service#971 specifies POST /api/sites/{siteId}/config-sync, authorised on site admin for the site. This sends POST /environments/{environmentId}/config-import — wrong resource and wrong path.
determineSiteInstance() already resolves $siteInstance->site (CommandBase.php:895), but execute() keeps only the environment (line 112). Pass $siteInstance->site->id.
| // @todo DXBE-20: Confirm the operation ID field name with the SAS team. | ||
| $operationId = $response->id ?? null; |
There was a problem hiding this comment.
acquia/sites-aggregation-service#971 returns 202 Accepted with a Message and no id, so this throws an exception on every invocation. testExecuteThrowsWhenOperationIdMissing() is currently testing the production path.
Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 asks for config-sync to create a notification. If it does, the identifier comes off that notification rather than off the 202 body, so this block still has to be rewritten — it just stops being unfixable.
| * @todo DXBE-20: Confirm the status field name and its values with the | ||
| * SAS team. Assumes a `status` field mirroring the task gateway's | ||
| * phases (pending/running/succeeded/failed). | ||
| */ | ||
| private function waitForOperation(SourceConfig $sourceConfig, string $operationId): bool |
There was a problem hiding this comment.
GET /config-operation/{id} doesn't exist. acquia/sites-aggregation-service#971's description: "No notification record is created and no status polling is wired up, so the caller gets a 202 and no way to observe the outcome beyond the task log."
So getStatus(), waitForOperation() and the pending/running/succeeded/failed states are all guesses.
There is a real surface to aim at, though. SAS already exposes GET /sites/{siteId}/notifications, with status, progress and completed_at on each Notification — and Site::createBackup(), restoreBackup() and export() all create one. syncConfig() is the exception. So the shape is polling notifications once config-sync creates one, not a bespoke /config-operation/{id} resource.
Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 decides which: if config-sync creates a notification, this polls that instead of /config-operation/{id}; if fire-and-forget is the intended contract, this method comes out entirely. Either way the code changes.
| * @todo DXBE-20: Confirm the endpoint paths and response field names with the | ||
| * SAS team. The SAS endpoints do not exist yet; paths here are placeholders. | ||
| */ | ||
| class SourceConfig extends CloudApiBase | ||
| { | ||
| /** | ||
| * Trigger a config import on a site environment (repo to CMS). | ||
| * | ||
| * @return object The decoded response, expected to contain an operation ID. | ||
| */ | ||
| public function push(string $environmentId): object | ||
| { | ||
| return $this->client->request('post', "/environments/$environmentId/config-import"); | ||
| } |
There was a problem hiding this comment.
DXBE-19 asks for the current folder of config to be sent as the POST body. Per acquia/haas-drupal#2233, dr source:config:import --single-yaml reads exactly that from stdin — and the assembly this PR originally had (keyed by collection, then config name) matched it.
Commit e44a7a1 deleted that to match acquia/sites-aggregation-service#971's input: false. The result is a push that sends nothing from the developer's machine and imports whatever is already deployed. So pull → edit → push silently ignores local edits until they're committed and deployed.
Needs upstream first, then still changes here. Point 1 of my review on acquia/sites-aggregation-service#971 asks for an optional payload; if it lands, the deleted assembly has to be restored in this PR. If the answer is that the payload path is out of scope for DXBE-20, then DXBE-19 needs revisiting rather than this code.
| $filesystem = new Filesystem(); | ||
|
|
||
| // Wipe the directory so the local files mirror the remote state. | ||
| $filesystem->remove($configDir); |
There was a problem hiding this comment.
Pull is DXBE-8 scope, so not out of place — but acquia/sites-aggregation-service#971 adds only the import direction, so config-export and /payload have nothing to run against.
Not covered by my review on acquia/sites-aggregation-service#971 either — that asks about the push payload, not a config export endpoint. POST /sites/{siteId}/exports does exist, but it's a whole-site export (MysqlSiteExportAdapter), not CMS config. The export endpoint is DXBE-21, so this half is blocked on that landing first.
That matters because pull is the half that deletes files. testWritePayloadCreatesConfigDirWhenPayloadEmpty() shows it as a passing test: {} parses to [], passes the is_array() guard at line 65, wipes .acquia/config/ and writes nothing back. Any SAS-side bug returning an empty export destroys uncommitted local config, and the prompt says nothing about local deletion.
Suggest splitting pull into its own PR. If it stays: name the directory in the prompt, write to a temp dir and swap, and refuse to wipe on an empty payload.
| { | ||
| $this->setDirAndRequireProjectCwd($input); | ||
|
|
||
| $siteInstance = $this->determineSiteInstance($input); |
There was a problem hiding this comment.
The PR description says site identity is resolved from the git remote with no arguments needed. determineSiteInstance() (CommandBase.php:884-906) reads only --siteInstanceId and otherwise returns null, so line 106 throws unless that option is passed. acceptEnvironmentId() (line 69) registers an argument nothing reads.
CommandBase::determineEnvironment() (CommandBase.php:690-708) is the existing ladder: --siteInstanceId → environmentId argument → codebase environment → git-remote-matched application → prompt. It also normalises through EnvironmentTransformer, which would make commit 4768081 unnecessary.
All six execute() tests pass --siteInstanceId, which is why this wasn't caught.
| protected function operationLabel(): string | ||
| { | ||
| return 'Importing configuration'; | ||
| } |
There was a problem hiding this comment.
operationLabel() is the whole of the push wording, and ConfigCommandBase.php:116 renders it as Importing configuration on the <env> environment?. Per acquia/sites-aggregation-tasks#261, confirming it puts the site in maintenance mode, backs up the database, checkpoints config, imports, and on failure reverts the checkpoint or restores the database. acquia/sites-aggregation-service#971 opens its risk section with "This triggers a destructive operation on a live site … The site is offline for the duration."
Someone pointing this at production can't tell from that sentence that the site goes down. EnvMirrorCommand.php:61 and PushDatabaseCommand.php:48 are the models — they name what gets overwritten. Please say that the site goes into maintenance mode and is unavailable while this runs, and that a database backup is taken first.
Anchoring here because the warning is push-specific: pull doesn't take the site offline. But the base's sprintf('%s on the %s environment?', ...) template only has room for a short label, so either that grows or the base needs a hook for the extra warning text.
wimleers
changed the title
source:link, source:cms:config:pull and source:cms:config:push commandssource:link, source:unlink, source:cms:config:pull and source:cms:config:push commands
r4007825810 asked about the decode() flags. \Drupal\Component\Serialization\Yaml::decode() passes Yaml::PARSE_CUSTOM_TAGS, added for services.yml's !tagged_iterator (https://www.drupal.org/node/3436859), and could reasonably also pass Yaml::PARSE_CONSTANT, for !php/const/!php/enum in config and service YAML (https://www.drupal.org/node/3403883). Neither applies here: what this pulls is built from plain config storage reads plus three literal system.site strings, never a tagged or enum value, so decode() doesn't need to accept either. PARSE_CONSTANT would also be actively unsafe to add: a !php/const tag would name a class from the Source site's own Drupal codebase, never autoloadable from acli's standalone process, so it could only fail or resolve to an unrelated value from this machine. Parsing either tag without its flag throws a clear ParseException, which is correct for config that should never contain one. Documented this on decode() itself, with @see references to \Drupal\Core\Config\FileStorage and \Drupal\Component\Serialization\Yaml — the actual Drupal codebase this is meant to match byte for byte, so a future reader can check both sides directly instead of trusting the commit message. The one existing test exercising a custom tag was synthetic, not drawn from the real fixture (grep confirms zero tags in tests/fixtures/source-config); replaced it with a rejection test covering both tag forms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The full-command-list snapshot test never got the new command added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
ℹ️ Proposed future docs to be published at https://docs.acquia.com/acquia-cli/ now in the PR body 😊 Note the use of the native |
|
Didn't do a full re-read, but I did look at the commits since my last read-through, and I'm happy with this. This is merely awaiting resolution of the merge-blocking comments (#2035 (comment) and #2035 (comment)). |
Reports the site's latest import (GET /source-sites/{id}/config/import)
without starting one: no confirmation, no .acquia/config requirement, no
PUT. Reuses reportImport()'s existing succeeded/refused/failed reporting,
adding the import's started_at (per api-specs' SourceSiteConfigImport
schema, since nothing hands back an import's id when it starts, so
started_at is what lets a caller tell a reported import apart from its
own) and a distinct exit code for "running" so a CI/CD run can poll on it
without misreading in-progress as failure.
Extracted reportResult() so the json-output silencing and the trailing
json_encode aren't copy-pasted between the --status branch and the
existing push flow.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
New commit (ba0d14b), based on @justafish's "import/push status?" feedback: |
|
For the record, to test this manually then it needs a |
|
Current status: blocked on a release of Cloud API v3 to staging. Once that's done, this can be manually tested against a Source CMS site in staging, and if that passes muster, we can merge this. |
|
Still blocked on deployment. @justafish and I have been working to get this unblocked upstream. https://github.com/acquia/api-gateway/pull/83#issuecomment-6143043100. |
@phenaproxima, can you test this E2E based on this? 🙏 |
|
My results from manual testing... After successfully authenticating with ACLI (
I did, however, see the intended test site in the list. So I chose that, and it confirmed success: Next, I ran It sat silent for a while. A disconcertingly long time, with no output -- 30 seconds, maybe? It should surely show a spinner or something. But eventually, it ended with a success message: Inspecting I made a single change: renaming the Document media type. I did it surgically, with Next step: That's a useless error. I wonder what value is NULL? That shouldn't be the case at all, I changed nothing about the config except that one single item in that one single file. So this is a bug, and there's nothing in the error to clarify what's bad and what I should fix. |
That is a validation error. That's lacking the structured JSON output that would make for a much more legible validation error. That work was already in progress, and it's not yet in. However, @justafish was able to do a successful pull + push, so she suspects it's a bug in this PR's https://github.com/phenaproxima/cli/blob/DXBE-20-source-config-push/src/Helpers/SourceConfigDocument.php. So next steps:
|
SaveSourceSiteConfigRequest now requires a top-level source discriminator (request or github); without it the PUT returned "This value should not be null." instead of accepting the configuration. This command always sends the document itself, never GitHub, so source is always request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
So after @wimleers's fix in 96d9d5f, I tried to push again. SUCCESS! The push churned for about a minute, but it had a nice spinner. Once it finished, I checked the result in the Source site, and confirmed it appeared. I tried a second time, changing the label of the media type again, and it worked again. EXCELLENT! To finish my testing, I ran So: far as I can tell, this works and is good to go, despite the rough edges I noted in my first pass. |
|
🕵️ Root cause determined: the OpenAPI spec + expected request body for 🤔 But the error message clearly is suboptimal, because it doesn't indicate at all where the validation error occurred exactly. How is that possible? 🚨 The Cloud Platform API does report which property failed, but neither the typhonius/acquia-php-sdk-v2 SDK that
✅ Fixed in Out of scope: |
DXBE-71 added source (request or github) to the site's latest import: which write started it, the same two values the save request takes. --status's text output names it alongside started_at, since it is the one field that can tell a developer their own push apart from the site's GitHub Two-Way Sync having just run one on its own. The API's own values are internal names; a developer reading the text output sees "ACLI request" or "GitHub commit" instead. Left out of reportImport()'s normal push-flow reporting: a push always sends source: request, so it would be the same word on every run and say nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
You've re-discovered a known bug in an internal API: |
…alid Reported by Adam Hoenich's testing: passing a site ID still prompted the chooser. `if ($siteId = $input->getArgument(...))` reads an empty string the same as no argument at all, since PHP truthiness treats both as false, so an empty argument fell through to the "no argument" branch and opened the interactive chooser instead of failing on it. Checking for null instead routes an empty string into the existing character-set validation, which already rejects it (the regex requires at least one character), so no new error message was needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Investigated. The code in However, you wrote that you ran 2089b58 (already in before this) makes an empty value fail with a clear error instead of opening the chooser, in case the variable was set but empty rather than unset. |
…-push * origin/main: Bump the dependencies group with 3 updates (acquia#2052)
|
Status on the two next steps (#2035 (comment)):
|
|
CI went red today on an unchanged lockfile: Rather than bump these ourselves, waiting for dependabot to file its own PR, same as e.g. acquia/cli#2052. |
Adds four commands for moving a Source site's configuration between the live site (served by Acquia) and a working copy (a local directory containing
.acquia/config, possibly managed in version control):acli source:link [<sourceSiteId>].acquia-cli.yml, likeapp:linkdoes for a Cloud applicationacli source:unlinkapp:unlinkacli source:cms:config:pullGET /source-sites/{id}/configon Cloud API v3, splits the returned document into.acquia/config/, one file per configuration object, replacing the directoryacli source:cms:config:push.acquia/config/back into one document,PUTs it, then pollsGET /source-sites/{id}/config/importuntil it issucceeded,refused(every violation is printed) orfailed; the exit code followsEndpoints used, all on Cloud API v3 (
https://api.acquia.com/v3,ACLI_CLOUD_API_V3_BASE_URIto override) with the same credentials every otheraclicommand uses:source:linkGET /source-sites(chooser, interactive only) andGET /source-sites/{sourceSiteId}getSourceSites,findSourceSiteBySourceSiteIdsource:cms:config:pullGET /source-sites/{sourceSiteId}/configfindSourceSiteConfigBySourceSiteIdsource:cms:config:pushPUT /source-sites/{sourceSiteId}/config, thenGET /source-sites/{sourceSiteId}/config/importsaveSourceSiteConfigBySourceSiteId,findSourceSiteConfigImportBySourceSiteIdThe working copy is the directory containing
.acquia/config; the commands work from any subdirectory of it. A site is given with--siteor taken from.acquia-cli.yml; nothing is guessed. Push asks for confirmation when interactive and requires--yesotherwise;--format=jsonprints the import outcome as JSON.The one piece of logic
acliowns is the conversion between the document and the directory (SourceConfigDocument). Files are written with the same YAML dumper settings Drupal core uses, andtests/fixtures/source-configholds a document a site emitted next to the files the site itself writes for it; the tests check both directions byte for byte.That fixture is most of this diff:
tests/fixtures/source-config/alone is 131 files and 11,732 lines. It is the complete configuration of a freshly installed Source site, written by the site's own configuration storage, not a hand-picked subset: the point is that a working copyacliwrites is indistinguishable from one the site writes, and only the site's full output proves that for every configuration type in scope. The README there records how to regenerate it when the scope changes.Caution
Two blockers:
search_api_taskfixture is merge-blocking: it's a real export bug tracked in DXBE-67, not a mistake in this PR's fixture generation. Once fixed, re-running the fixture export (seetests/fixtures/source-config/README.md) should drop the file on its own.Note
History: the first 15 commits are @phenaproxima's original draft against placeholder endpoints; the commits after the merge of
mainreplace the hand-written service client with the existingV3ClientService, rename the commands, add the transform andsource:link/source:unlink, and drop two decode() flags (Yaml::PARSE_CUSTOM_TAGS/PARSE_CONSTANT) that core's own YAML class carries for unrelated reasons.Not yet done
Proposed docs for
https://docs.acquia.com/acquia-cli/https://dev.acquia.com→ moved to DXBE-114's https://github.com/acquia/developer-portal/pull/45